Docs: fill in TODOs and improve cross-links#33263
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d3806a574
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cmpadden
left a comment
There was a problem hiding this comment.
Thanks @Vamsi-klu , however, we should not be removing all of the TODOs without replacing them with valid content.
If you would like to undo those removals but keep the content additions that would be appreciated.
|
@cmpadden Thanks for your comment, i will undo this and raise a PR for your review shortly in 2 days. Thanks! |
Addresses @cmpadden review feedback on dagster-io#33263: - dagster-omni.rst: the restored TODO was concatenated onto the link sentence with no newline, turning the whole line into a hidden RST comment. The state-backed components guide now exists, so this replaces the TODO with the proper visible link (valid-content replacement, per the review request). - dagstermill examples/repository.py: restore the dropped "# FIXME add an output to this" marker (the op still has no output). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @cmpadden. The dropped TODO placeholders were restored across the docs in an earlier push, and I just fixed two remaining issues from your review:
Two honest notes: this PR still has merge conflicts with master, and it mixes docs cleanups with some larger core-logic changes (asset_backfill pagination, run_monitoring batching, child-process stdout/stderr capture). If it's easier to review, I'm happy to split the docs-only changes into a separate, smaller PR and pull the core-logic changes out into their own focused PRs. Let me know what you'd prefer. |
Greptile SummaryDespite being titled "Docs: fill in TODOs and improve cross-links", this PR also lands a significant batch of Python backend improvements that resolve long-standing TODOs: streaming pagination for asset-key merging, child-process stdout/stderr capture, bulk dynamic-partition operations,
Confidence Score: 3/5Two backend changes introduce real correctness problems: the CLI-log event parser can silently lose DagsterEvents, and the new BigQuery table_definitions config is effectively non-functional due to a key-format mismatch. Everything else in the PR is safe. The event-parser rewrite in
|
| Filename | Overview |
|---|---|
| python_modules/dagster/dagster/_core/events/utils.py | Rewrites filter_dagster_events_from_cli_logs using json.JSONDecoder.raw_decode for proper multi-line JSON handling, but introduces a buffer-accumulation regression where non-JSON text with { can prevent subsequent valid events from being parsed. |
| python_modules/libraries/dagster-gcp/dagster_gcp/bigquery/ops.py | Implements previously TODO'd table_definitions and udf_resources preprocessing, but ExternalConfig.from_api_repr requires camelCase API keys while the config schema emits snake_case, so table_definitions configs are silently dropped. |
| python_modules/libraries/dagster-gcp/dagster_gcp/bigquery/configs.py | Fills in previously TODO'd table_definitions and udf_resources fields with snake_case schema keys; the mismatch with ExternalConfig.from_api_repr's camelCase expectations makes table_definitions non-functional in practice. |
| python_modules/dagster/dagster/_core/executor/child_process_executor.py | Adds stdout/stderr capture to child processes via temp files and a _TeeStream tee; enriches ChildProcessCrashException and ChildProcessSystemErrorEvent with captured output. Minor file-handle ordering issue in _redirect_output. |
| python_modules/dagster-graphql/dagster_graphql/implementation/fetch_assets.py | Replaces in-memory full-fetch-then-merge with streaming heapq.merge over sorted asset-graph and paginated materialized-key iterators; correct deduplication and limit handling. |
| python_modules/dagster/dagster/_core/execution/asset_backfill.py | Replaces list(subset.get_partition_keys())[:N] with paginated _get_first_partition_keys_in_subset, resolving a TODO about not enumerating all keys upfront. |
| python_modules/dagster/dagster/_daemon/monitoring/run_monitoring.py | Adds pagination to run-record fetching via _get_run_record_batches; workspace context is now created unconditionally before the first batch, which is a minor inefficiency when no runs need monitoring. |
| python_modules/dagster/dagster/_daemon/sensor.py | Replaces per-key has_dynamic_partition loops with single-query get_dynamic_partitions_by_keys and bulk delete_dynamic_partitions, resolving two TODOs. |
| python_modules/dagster/dagster/_core/storage/event_log/sql_event_log.py | Adds get_dynamic_partitions_by_keys (single IN query) and delete_dynamic_partitions (bulk DELETE ... WHERE ... IN) to the SQL event log. |
| python_modules/libraries/dagster-spark/dagster_spark/types.py | Introduces SparkMemory and SparkTime validated config types using regex post-processing, replacing the prior StringSource placeholders for Spark memory/time fields. |
| python_modules/dagster/dagster/_core/asset_graph_view/asset_graph_view.py | Replaces expensively_compute_asset_partitions with get_latest_storage_id_by_partition for computing missing subsets, avoiding a full scan of all partitions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_assets GraphQL resolver] --> B{asset_keys provided?}
B -- No --> C[_iter_asset_graph_keys\nsorted cached property]
B -- No --> D[_iter_materialized_keys\npaginated DB fetch]
C --> E[heapq.merge\nsorted by key_str]
D --> E
E --> F{key_str == last_key?}
F -- Yes, duplicate --> G[skip]
F -- No --> H[append to merged_asset_keys]
H --> I{len >= limit?}
I -- Yes --> J[return GrapheneAssetConnection]
I -- No --> E
B -- Yes --> K[use provided asset_keys directly]
K --> J
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[get_assets GraphQL resolver] --> B{asset_keys provided?}
B -- No --> C[_iter_asset_graph_keys\nsorted cached property]
B -- No --> D[_iter_materialized_keys\npaginated DB fetch]
C --> E[heapq.merge\nsorted by key_str]
D --> E
E --> F{key_str == last_key?}
F -- Yes, duplicate --> G[skip]
F -- No --> H[append to merged_asset_keys]
H --> I{len >= limit?}
I -- Yes --> J[return GrapheneAssetConnection]
I -- No --> E
B -- Yes --> K[use provided asset_keys directly]
K --> J
Reviews (1): Last reviewed commit: "docs: fix omni state-backed link; restor..." | Re-trigger Greptile
| try: | ||
| _, end_idx = decoder.raw_decode(trimmed) | ||
| except JSONDecodeError: | ||
| buffer = trimmed | ||
| break |
There was a problem hiding this comment.
Buffer accumulation silently drops valid DagsterEvents
When raw_decode fails (incomplete or malformed JSON beginning with {), the code sets buffer = trimmed and accumulates subsequent lines onto it. If any log line contains a { that isn't the start of complete JSON — for example an exception message like ValueError: Expected dict {"key": "val"} split across lines — the partial JSON anchors the buffer. All subsequent log_lines are appended to that stuck buffer. A valid DagsterEvent JSON object appearing on a later line then becomes inaccessible because raw_decode is applied to the combined string starting from the earlier invalid {, not the new one.
The original code avoided this by only entering accumulation mode when the line started with { and only ever appended to the buffer while in_split_line was True. The new code finds any { in the line and never advances past a failed parse attempt. Minimal fix: when raw_decode fails on a {-prefixed slice, advance the buffer by 1 character and continue the inner while loop rather than breaking out to accumulate more lines.
| if table_definitions is not None: | ||
| try: | ||
| from google.cloud.bigquery.external_config import ExternalConfig | ||
| except ImportError: | ||
| pass | ||
| else: | ||
| cfg["table_definitions"] = { | ||
| name: ExternalConfig.from_api_repr(defn) if isinstance(defn, dict) else defn | ||
| for name, defn in table_definitions.items() | ||
| } |
There was a problem hiding this comment.
table_definitions config silently ignored due to snake_case / camelCase mismatch
ExternalConfig.from_api_repr expects the BigQuery REST API wire format with camelCase keys (sourceUris, sourceFormat, autodetect, schema). The Dagster config schema defined in configs.py uses snake_case keys (source_uris, source_format). When a user sets source_uris: [...] in their Dagster config, that dict is passed directly to from_api_repr as {"source_uris": [...]}. The library will silently ignore the unrecognised key and return an ExternalConfig with no source URIs set — the external table definition is effectively empty with no error. The fix is either to rename the schema fields to camelCase (sourceUris, sourceFormat) to match the REST API representation, or to translate from snake_case to camelCase before calling from_api_repr.
| stdout_file = open(stdout_path, "w", encoding="utf-8") if stdout_path else None | ||
| stderr_file = open(stderr_path, "w", encoding="utf-8") if stderr_path else None | ||
| try: |
There was a problem hiding this comment.
stdout_file is opened before the try block. If the second open(stderr_path, ...) call raises (e.g., due to an unexpected OS error), stdout_file is left open and never closed. Both opens should be inside the guarded region.
| stdout_file = open(stdout_path, "w", encoding="utf-8") if stdout_path else None | |
| stderr_file = open(stderr_path, "w", encoding="utf-8") if stderr_path else None | |
| try: | |
| stdout_file = None | |
| stderr_file = None | |
| try: | |
| stdout_file = open(stdout_path, "w", encoding="utf-8") if stdout_path else None | |
| stderr_file = open(stderr_path, "w", encoding="utf-8") if stderr_path else None |
| // TODO_USE_CHECK_ID | ||
| const rowTestId = `row-${assetNode.assetKey.path.join('-')}-${row.name}`; |
There was a problem hiding this comment.
The
// TODO_USE_CHECK_ID comment is no longer actionable here — the variable name and the surrounding code already communicate the intent. Keeping it risks misleading future readers into thinking rowTestId is still a placeholder.
| // TODO_USE_CHECK_ID | |
| const rowTestId = `row-${assetNode.assetKey.path.join('-')}-${row.name}`; | |
| // TODO: replace with the actual check ID once it is available on the row type | |
| const rowTestId = `row-${assetNode.assetKey.path.join('-')}-${row.name}`; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Closing in favor of #33940. Per the earlier discussion about splitting this, I pulled the conflict-free docs improvements out into #33940 — a small, focused docs-only PR against current master. That's the part that was ready and reviewable. The remaining core-logic changes (asset_backfill pagination, run_monitoring batching, child-process stdout/stderr handling, etc.) had merge conflicts and some flagged correctness concerns, so rather than carry them in a sprawling 58-file PR I'll raise any that are still worth doing as separate, focused PRs. Thanks @cmpadden for the review! |
Salvaged the conflict-free docs subset from dagster-io#33263 onto current master as a focused, reviewable docs-only change. Replaces TODO/placeholder markers with valid prose, wires up PyObject API references, and corrects several internal links across the build/test guides and the GCP/Snowflake integration docs.
Summary & Motivation
How I Tested These Changes